-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Support DGCNN #801
Conversation
Codecov Report
@@ Coverage Diff @@
## master #801 +/- ##
==========================================
+ Coverage 49.32% 49.47% +0.14%
==========================================
Files 210 216 +6
Lines 16019 16179 +160
Branches 2556 2574 +18
==========================================
+ Hits 7902 8004 +102
- Misses 7614 7670 +56
- Partials 503 505 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code needs to be refined. Benchmark is required. I think the design of DGCNN is acceptable (via ops/dgcnn_modules
), but we need to polish the code. Also, unit tests are needed. See my comments above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for results on 6-fold evaluation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZwwWayne Please have a look here. The benchmark is done on S3DIS (both Area 5 and 6-Fold). The code in each added module is good. However, I don't know if you think the overall design of DGCNN is acceptable (I think it's okay). Similar to PointNet++, in this repo, DGCNN is decomposed to several sub-modules, including GraphFeature (gf
), FeatureAggregation (fa
) and FeaturePropagation (fp
). Please kindly have a look.
@DCNSW Please kindly check the docstring of the code in this PR. Add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is almost ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job. Cheers!
Considering reduce the registry of fa/fg module |
Some benchmark release related modifications are missed. Please refer to #541 for more details, e.g. adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also fix minor problems similar to our comments
Motivation
Support DGCNN (Dynamic Graph CNN for Learning on Point Clouds) in mmdetection3d.
Modification
Design new configs, backbones, decode_heads and ops for DGCNN.
BC-breaking (Optional)
Does the modification introduce changes that break the back-compatibility of the downstream repos?
No.
Checklist